Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[spi_device] Remove generic mode #20856

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Jan 17, 2024

Remove generic mode from spi_device.

Generic mode has a very different timing model than the relatively high throughput, high latency flash and tpm modes. Generic mode requires data to be available on the MISO pin before the first SCK edge, whereas flash and tpm modes have a significant command phase that comes first. In addition, generic mode requires support for a number of different idle clock polarities and sampling phases, whereas flash and TPM modes have one clearly defined set.

The complexity for supporting both timing models for one IP led to a compromised performance for flash and TPM. Remove generic mode here to permit optimizations that target the latter modes.

Resolves #15452

@a-will a-will force-pushed the spid-remove-fwmode branch 3 times, most recently from 9e1cbf9 to 4c901c8 Compare January 17, 2024 04:11
@a-will a-will force-pushed the spid-remove-fwmode branch 4 times, most recently from a2f6535 to f153f91 Compare January 19, 2024 22:24
@a-will
Copy link
Contributor Author

a-will commented Jan 19, 2024

Sorry for the massive PR, but I'm hoping the commit breakdown will help direct folks to the areas that they're most concerned about. This is primarily deletion, with some swaps where appropriate (primarily for looping from the start of the interrupt block).

Here's the plan: Once each of the areas of DD, DV, and SW are represented, I'll squash all the commits down to one, write some reasonable error message, then seek a final approval with this PR set to no longer be a draft. The diff for the squashed commit should be empty, and that last approval will merely be about the commit message, hehe.

Edit: The first commit now has the intended commit message, so I've pulled this out of draft state. The other commits will all be squashed into the first once reviews are complete.

"spi_device_dummy_item_extra_dly_vseq",
"spi_device_intr_vseq",
"spi_device_perf_vseq",
string seq_names[$] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"stress_all" doesn't really stress much anymore, does it? 😁

Comment on lines +42 to +45
tags = [
"broken",
"manual",
],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Tock test is disabled for now, but most likely, this isn't the best repo for it. I believe RTL changes in this repo should not be gated by a simultaneous Tock support requirement. In terms of scheduling and resources, Tock support is downstream (and probably should depend on this repo's releases, rather than keeping pace with each PR).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this deserve a comment inline so that it is clear why this has been marked disabled/broken?

Copy link
Contributor Author

@a-will a-will Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I think we should remove this test from the repo altogether, really.

It's broken because a third-party software repo doesn't support earlgrey*, but it seems a little funky to have this repo carry issues for that one. Tock is important to us, but I believe it should be positioned downstream of this repo.

*I'm being a little obtuse with the naming here, but from the perspective of the master branch, what was called "earlgrey ES" is left to history.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with removing it, since it creates a weird dependency loop.

@a-will a-will force-pushed the spid-remove-fwmode branch from f153f91 to f916d6b Compare January 20, 2024 00:35
@a-will a-will marked this pull request as ready for review January 20, 2024 00:37
@a-will a-will requested review from a team as code owners January 20, 2024 00:37
@moidx moidx removed request for a team January 20, 2024 01:14
@engdoreis engdoreis requested a review from nbdd0121 January 22, 2024 10:23
@a-will
Copy link
Contributor Author

a-will commented Jan 22, 2024

CC @mazurek-michal

Copy link
Contributor

@msfschaffner msfschaffner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the heavy lifting here. I just have a few questions/remarks.

hw/ip/spi_device/data/spi_device.hjson Show resolved Hide resolved
Comment on lines +42 to +45
tags = [
"broken",
"manual",
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this deserve a comment inline so that it is clear why this has been marked disabled/broken?

hw/ip/spi_device/rtl/spi_device.sv Show resolved Hide resolved
Copy link
Contributor

@engdoreis engdoreis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SW side LGTM. Thanks for working on this.


//////////////////////
// Interrupt Enable //
//////////////////////
for (genvar s = 0; s < 185; s++) begin : gen_ie0
for (genvar s = 0; s < 179; s++) begin : gen_ie0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silly question, I've seen this number repeating throughout the code base, Isn't there a easy way to get this number from the hjson file?

Copy link
Contributor Author

@a-will a-will Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean, specifically. This file is generated by topgen / ipgen, and this number already comes from the top-level hjson file.

However, if you are saying that stylistically, it'd be better if the rv_plic ipgen templates used a named constant throughout instead of a magic number, then I do agree!

(And really, the constant is already available. ${module_instance_name}_reg_pkg::NumSrc is that number)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik, this code is generated (so all these numbers should be automatically updated through the template).
We could make it look nicer by converting to parameters I agree - but the numbers are not hand edited.

Remove generic mode from spi_device.

Generic mode has a very different timing model than the relatively high
throughput, high latency flash and tpm modes. Generic mode requires data
to be available on the MISO pin *before* the first SCK edge, whereas
flash and tpm modes have a significant command phase that comes first.
In addition, generic mode requires support for a number of different
idle clock polarities and sampling phases, whereas flash and TPM modes
have one clearly defined set.

The complexity for supporting both timing models for one IP led to a
compromised performance for flash and TPM. Remove generic mode here to
permit optimizations that target the latter modes.

Signed-off-by: Alexander Williams <awill@opentitan.org>
@a-will a-will force-pushed the spid-remove-fwmode branch from f916d6b to 30ff93f Compare January 23, 2024 22:48
@a-will a-will merged commit 726cd46 into lowRISC:master Jan 23, 2024
2 of 9 checks passed
@a-will a-will deleted the spid-remove-fwmode branch January 23, 2024 22:48
@a-will a-will mentioned this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[spi_device] Remove Generic (a.k.a. FwMode) from SPI_DEVICE HWIP
4 participants